Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 12 files
Confidence score: 2/5
- There are high-confidence, user-impacting issues here, so merge risk is elevated despite the changes being reviewable overall.
- Most severe: in
internal/repository/repository.go,getNotificationRaw+encryptNotificationSecretscan double-encrypt unchanged secret fields during partial updates, which can corrupt stored credentials and break downstream decrypt/use flows. - In
internal/notifications/dispatcher.go, invalid SMTP security values can fall through to effectively unencrypted behavior, creating a concrete security regression risk when configs contain typos or unexpected values. - Pay close attention to
internal/repository/repository.goandinternal/notifications/dispatcher.go- secret handling and SMTP transport security behavior need fixes before merging.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/notifications/dispatcher.go">
<violation number="1" location="internal/notifications/dispatcher.go:59">
P1: Unrecognized SMTP security values silently fall back to an unencrypted connection. The `default` case returns the raw string, which won't match `"ssl_tls"` or `"starttls"` in downstream checks, so a typo (e.g. `"starttl"`) sends credentials and email content in plaintext. Return an error for unrecognized values instead.</violation>
</file>
<file name="internal/repository/repository.go">
<violation number="1" location="internal/repository/repository.go:335">
P0: Double-encryption bug: `getNotificationRaw` returns already-encrypted secrets, and then `encryptNotificationSecrets` re-encrypts every non-empty secret field — including those not being patched. Each update that doesn't touch `DiscordWebhookURL` or `SMTPPassword` will double-encrypt the stored value, corrupting it permanently.
Decrypt `existing` before applying patches (or selectively encrypt only patched fields), mirroring how `UpdateConnection` handles passwords.</violation>
<violation number="2" location="internal/repository/repository.go:448">
P1: `created_at` and `updated_at` are missing from the map. GORM does not auto-populate timestamps for `map[string]any` creates via `tx.Table().Create()` — only for struct-based creates. These columns will be NULL/zero, breaking the `ORDER BY created_at asc` in `ListBackupNotifications`.
Add timestamp entries to the map, or use a struct with `tx.Omit(clause.Associations).Create(&models.BackupNotification{...})`.</violation>
</file>
Architecture diagram
sequenceDiagram
participant User as User / UI
participant API as API Handler
participant Repo as Repository
participant DB as Database (SQLite/GORM)
participant Sched as Scheduler
participant Disp as Notification Dispatcher
participant Ext as External (Discord/SMTP)
Note over User,Ext: Notification Configuration Flow
User->>API: POST /notifications (with secrets)
API->>Repo: NEW: CreateNotification()
Repo->>Repo: NEW: Encrypt secrets at rest
Repo->>DB: NEW: Insert NotificationDestination
DB-->>Repo: Saved
Repo-->>API: Return model
API->>API: NEW: Redact secrets in response
API-->>User: 201 Created (Redacted)
Note over User,Ext: Manual Test Flow
User->>API: POST /notifications/{id}/test
API->>Repo: GetNotification(id)
Repo->>Repo: NEW: Decrypt secrets
Repo-->>API: Notification with raw secrets
API->>Disp: NEW: SendTestNotification(dest)
Disp->>Ext: NEW: POST Webhook or SMTP Dial
Ext-->>Disp: Success/Failure
Disp-->>API: Result
API-->>User: 200 OK / 502 Bad Gateway
Note over User,Ext: Automated Backup Flow (Scheduled)
Sched->>Sched: Trigger Backup Task
Sched->>Repo: FinishBackupRun(status, error)
rect rgb(240, 240, 240)
Note right of Sched: NEW: Post-Run Notification Logic
Sched->>Disp: NEW: NotifyBackupRun(backup, run)
Disp->>Repo: NEW: ListNotificationDestinationsForEvent(id, status)
Repo->>DB: Query BackupNotification bindings
DB-->>Repo: Results
Repo->>Repo: NEW: Decrypt secrets
Repo-->>Disp: List of active destinations
loop For each Destination
alt Type: Discord
Disp->>Ext: NEW: POST webhook payload
else Type: SMTP
Disp->>Ext: NEW: STARTTLS/SSL + Auth + Send
end
Ext-->>Disp: Response
end
Disp-->>Sched: Complete (errors logged, not bubbled)
end
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by cubic
Adds notifications to AnchorDB with Discord webhooks and SMTP, including per-schedule success/failure routing, test-send, and a new UI to manage destinations and bindings. The scheduler now sends notifications after each backup run, and SMTP security handling is normalized and validated to prevent misconfiguration.
New Features
internal/notifications.Dispatcherwith Discord webhook and SMTP support; SMTP security modes (STARTTLS, SSL/TLS, none) are normalized and strictly validated.POST /notifications,GET /notifications,GET /notifications/{id},PATCH /notifications/{id},POST /notifications/{id}/test,DELETE /notifications/{id}GET /backups/{id}/notifications,PUT /backups/{id}/notificationsfor per-schedule success/failure bindings.NotificationDestinationandBackupNotification; Discord webhooks and SMTP passwords are encrypted at rest.Migration
Written for commit f85c574. Summary will update on new commits.